-
-
Notifications
You must be signed in to change notification settings - Fork 442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(notebook): align actions and hotkeys #5353
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are three states of tab visibility
- Hide all tabs
- Show only tabs that have someone streaming in them
- Show all tabs (default)
What does it mean to toggle the tab visibility? imo it means you toggle between 1 and 3
What does it mean to toggle the tab live only state? imo it means you toggle between whatever state it was in before (1 or 3) and 2
Part of the confusion comes from having different menu actions to hotkeys, but it doesn't help that the hotkeys are confusing to begin with (2 toggle actions between 3 states??)
In addition to some of the changes you've done, I propose the tab visibility is moved to a submenu with 3 always visible options matching the three states of tab visibility.
- Hide all tabs (Linked to the hotkey Set tab visibility -> Set to off)
- Show live tabs only (Linked to the hotkey Set tab visibility -> Live only on)
- Show all tabs (Linked to the hotkey Set tab visibility -> Set to on)
The downside I see to the above-mentioned change is we no longer surface the default toggle hotkeys. This could be alleviated by adding 2 more options for the toggling
src/widgets/Window.cpp
Outdated
this->notebook_->setShowTabs(true); | ||
getSettings()->tabVisibility.setValue( | ||
NotebookTabVisibility::LiveOnly); | ||
this->notebook_->toggleOfflineTabsAction()->trigger(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change feels wrong - in the hotkey selector, the value for this is "Live only on".
Right below this there's a "toggleLiveOnly" which calls this->notebook_->toggleOfflineTabs()
That's what's happening: graph TD;
All<-->|toggleLive|LiveOnly
NoneAll<-->|toggleAll|All
NoneAll-->|toggleLive|LiveOnly
NoneLive-->|toggleAll|LiveOnly
NoneLive<-->|toggleLive|All
I'm unsure about this.
Should be "Tab Visibility" → {All, Live Only, None}.
This also breaks the workflow of right-clicking and clicking on the desired option, as you now have to move through the submenu.
|
Aligns Ctrl+U and "Toggle visibility of tabs" to do the same. The hotkey will now trigger the same
QAction
. I've done the same for Ctrl+Shift+L and removed the visibility filter reset when the hotkey is activated withon
oroff
.Fixes #5341.